Conversation
| card.getSpellAbility().addEffect(effect.copy()); | ||
| OverloadedEffect overloadEffect = new OverloadedEffect(effect, target.copy()); | ||
| overloadEffect.setText(effect.getText(card.getSpellAbility().getModes().getMode()) | ||
| .replace("target", "each")); |
There was a problem hiding this comment.
Why you don’t use conditional style effect and target (one pair for single and one pair for all)? Auto-replace looks overdeveloped and potentially buggy.
There was a problem hiding this comment.
The "all" version of the spell needs to have no target when cast. It sounds to me like you're recommending not continuing this rework and keeping the existing code with its fully separate effects.
The whole point of this rework is to remove the duplicated code by auto-replacing the targets.
There was a problem hiding this comment.
Yea, it’s looks strange in current implementation — one shot effect but it can be continues effect inside — thats can broke some effects/targets lifecycle in theory. Not critical and maybe workable.
Also inner effects must support work with target pointers (e.g. each inner effect must have special code to check and use the pointers). Must be carefully replaced and checked.
There was a problem hiding this comment.
I would not call it "special code" as all effects ought to use target pointers as the default, but I agree need to verify each effect properly uses them.
| protected OverloadedEffect(final OverloadedEffect effect) { | ||
| super(effect); | ||
| this.innerEffect = effect.innerEffect; | ||
| this.target = effect.target; |
|
I endorse this direction (just make sure there's adequate test coverage) |
3 participants
Casting a spell with Overload converts all "target" text to "each". Currently this is done by having entirely separate effects. Instead, we can apply the effects after setting up a
FixedTargets.I have not yet finished converting all Overload cards, once I do I will remove the old
OverloadAbilityconstructor. Currently only A-D cards have been modified. Several of the more complex Overload cards will require merging the target and overload effect implementations as the target version usesgetFirstTarget.I was wondering if this seemed worth finishing. I don't expect it to fix any bugs, just remove a lot of repetitive code.